Conversation
flakey5
force-pushed
the
flakey5/159
branch
2 times, most recently
from
0ade6ef to
7fb3816
Compare
|
cc @flakey5 some of the CI steps are failing |
ovflowd
left a comment
ovflowd
left a comment
There was a problem hiding this comment.
Didn't finish the review yet, but leaving initial comments here :)
![sentry[bot]](https://avatars.githubusercontent.com/in/12637?s=60&v=4){kind=small}
PR SummaryMedium Risk Overview New tooling automates populating/updating the KV directory cache. This replaces CI/config updates to support KV. Adds a Reviewed by Cursor Bugbot for commit 22dc867. Bugbot is set up for automated code reviews on this repo. Configure here. |
![github-code-quality[bot]](https://avatars.githubusercontent.com/u/9919?s=60&v=4){kind=small}
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 3 potential issues.
Autofix Details
Bugbot Autofix prepared fixes for all 3 issues found in the latest run.
- ✅ Fixed: Version argument crashes before null check executes
- I now validate the raw version argument before calling
toLowerCase()so missing input throws the intended explicit error.
- I now validate the raw version argument before calling
- ✅ Fixed: Subdirectories cached without trailing slashes in e2e tests
- The e2e cache population now appends
/to each subdirectory name so test data matches production directory-cache shape.
- The e2e cache population now appends
- ✅ Fixed:
latestkey incorrectly added as directory symlink- I restricted added directory symlinks to keys starting with
latest-, which excludes bothlatestandnode-latest.tar.gz.
- I restricted added directory symlinks to keys starting with
Or push these changes by commenting:
@cursor push 29199a84ef
Preview (29199a84ef)
diff --git a/e2e-tests/util.ts b/e2e-tests/util.ts
--- a/e2e-tests/util.ts
+++ b/e2e-tests/util.ts
@@ -47,7 +47,9 @@
});
const cachedDirectory: ReadDirectoryResult = {
- subdirectories: Object.keys(directory.subdirectories),
+ subdirectories: Object.keys(directory.subdirectories).map(
+ subdirectory => `${subdirectory}/`
+ ),
files,
hasIndexHtmlFile,
lastModified: new Date(),
diff --git a/scripts/update-directory-cache.mjs b/scripts/update-directory-cache.mjs
--- a/scripts/update-directory-cache.mjs
+++ b/scripts/update-directory-cache.mjs
@@ -20,12 +20,13 @@
import { createCloudflareClient, writeKeysToKv } from './utils/kv.mjs';
import { createS3Client, listR2DirectoryRecursive } from './utils/r2.mjs';
-const VERSION = process.argv[2].toLowerCase();
-
-if (!VERSION) {
+const VERSION_ARG = process.argv[2];
+if (!VERSION_ARG) {
throw new TypeError('version missing from args');
}
+const VERSION = VERSION_ARG.toLowerCase();
+
if (!VERSION.startsWith('v')) {
throw new TypeError(
'provided version not in correct format, expected vX.X.X'
diff --git a/scripts/utils/addSymlinksToDirectoryCache.mjs b/scripts/utils/addSymlinksToDirectoryCache.mjs
--- a/scripts/utils/addSymlinksToDirectoryCache.mjs
+++ b/scripts/utils/addSymlinksToDirectoryCache.mjs
@@ -55,8 +55,8 @@
) {
releaseDirectory.subdirectories.push(
...Object.keys(latestVersionMap)
- // We only want directories, remove the only file in the map
- .filter(version => version !== NODE_LATEST_FILE_NAME)
+ // We only want the `latest-*` directory symlinks.
+ .filter(version => version.startsWith('latest-'))
.map(version => `${version}/`)
);
releaseDirectory.subdirectories.sort();
diff --git a/scripts/utils/addSymlinksToDirectoryCache.test.ts b/scripts/utils/addSymlinksToDirectoryCache.test.ts
--- a/scripts/utils/addSymlinksToDirectoryCache.test.ts
+++ b/scripts/utils/addSymlinksToDirectoryCache.test.ts
@@ -97,6 +97,7 @@
const latestVersionMap: LatestVersionMapping = {
'latest-v20.x': 'v20.1.2',
'latest-v22.x': 'v22.2.1',
+ latest: 'v22.2.1',
'node-latest.tar.gz': 'latest/node-v22.2.1',
};This Bugbot Autofix run was free. To enable autofix for future PRs, go to the Cursor dashboard.
ovflowd
left a comment
ovflowd
left a comment
There was a problem hiding this comment.
As I said before, it is hard to review so many complex changes in a timely manner and one pass.
@flakey5 can you do me a favor? Can you address BugBot's reviews and maybe also ask Claude Code to scrutinize your code and do a 2nd pass in terms of code smell and what not? And then let's get this merged.
Re nodejs/release-cloudflare-worker#159 Re nodejs/release-cloudflare-worker#756 Signed-off-by: flakey5 <73616808+flakey5@users.noreply.github.com>
Signed-off-by: flakey5 <73616808+flakey5@users.noreply.github.com>
Re nodejs/release-cloudflare-worker#159 nodejs/release-cloudflare-worker#756 Signed-off-by: flakey5 <73616808+flakey5@users.noreply.github.com>
Re nodejs/release-cloudflare-worker#159 nodejs/release-cloudflare-worker#756 Signed-off-by: flakey5 <73616808+flakey5@users.noreply.github.com> PR-URL: #62777 Reviewed-By: Claudio Wunder <cwunder@gnome.org> Reviewed-By: Rich Trott <rtrott@gmail.com>
Signed-off-by: flakey5 <73616808+flakey5@users.noreply.github.com>
Signed-off-by: flakey5 <73616808+flakey5@users.noreply.github.com>
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 1 potential issue.
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit 22dc867. Configure here.
4 participants
Makes use of KV for directory listings. This aims to replace the
build-r2-symlinks.mjsfile with two different ones:build-directory-cache.mjs- This reads the entire dist-prod bucket and caches it into the KV namespace. This is pretty much only for the initial setup of the cache and for anytime we need to rebuild all of it.update-directory-cache.mjs- This reads only the necessary paths from the dist-prod bucket and updates them in the KV namespace. This will be the one that we will call in the workflowA
USE_KVenvironment variable is added to the worker that acts as a toggle between KV and S3 listings. This acts as a safeguard in case something goes wrong while we're still testing it, so we can easily switch between the two. Once we're more confident in KV, we can remove it + the entireS3Providerfrom the worker.Closes #159
Closes #314
TODO:
versionargument in Node core when invoking the update redirect links action: meta: pass release version to release worker node#62777